Avatar styles #175
Avatar styles #175
Conversation
✅ Deploy Preview for reablocks-storybook ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| /** | ||
| * Whether the avatar is able to interact. | ||
| */ | ||
| interactable?: boolean; |
There was a problem hiding this comment.
This should be inverse of disable. No need for new prop
There was a problem hiding this comment.
In some cases we don't neet user interaction with this component
There was a problem hiding this comment.
How is that different from disabled?
src/elements/Avatar/Avatar.tsx
Outdated
| @@ -24,7 +23,7 @@ export interface AvatarProps extends React.HTMLAttributes<HTMLDivElement> { | |||
| /** | |||
| * Style variant for the avatar. | |||
| */ | |||
| variant?: 'filled' | 'outline'; | |||
| variant?: 'filled' | 'outline' | 'colored'; | |||
There was a problem hiding this comment.
This variant name could be better. Its not obvious whats different here.
There was a problem hiding this comment.
Can we call this like primary or maybe secondary or something? this isn't a common name we use.
There was a problem hiding this comment.
primary or secondary it's more about color
There was a problem hiding this comment.
I think still no. Its not obvious what this is and how its different from the others. Like filled could mean colored imo.
There was a problem hiding this comment.
We had color property - now it is deprecated and moved to the variant. Also, user can pass colorOptions. IDK what you don't like.
There was a problem hiding this comment.
it seems like colored is the same things as filled, but with a diff background color. Is that right?
if a user wanted the avatar filled, but with a diff background color than their globally styled filled variant, it could be simpler for them to just pass an altered AvatarTheme to the component theme prop
There was a problem hiding this comment.
it seems like colored is the same things as filled, but with a diff background color. Is that right?
Yes
The reason was to make component similar to figma design without breaking the functionality. Let me rethink it
# Conflicts: # src/elements/Avatar/Avatar.story.tsx
|
I've moved it to the separate property |
src/elements/Avatar/Avatar.tsx
Outdated
| /** | ||
| * Style type for the avatar. | ||
| */ | ||
| type: 'colored' | 'monochrome'; |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Issue Number: GOOD-186
Does this PR introduce a breaking change?